The bootloader gets to big if all options are enables. How should we handle this? Maybe disable TWI by default but add an additional test where SPI is disabled instead?

(I'm gonna extend the documentation when we resolved this)

Add ability to read program from TWI

Nice! Thank you very much!

The bootloader gets to big if all options are enables.

Oh I know this problem... 🙈
How much more ROM do you need for your modified bootloader?

How should we handle this? Maybe disable TWI by default but add an additional test where SPI is disabled instead?

I was thinking about rewriting the bootloader in plain assembly... But to be honest, I'm too lazy for that... 😅

So yes, we could disable the TWI option by default and let the user decide if this is a relevant feature.

How much more ROM do you need for your modified bootloader?

Memory utilization:
   text    data     bss     dec     hex filename
   4456       0       8    4464    1170 main.elf

I was thinking about rewriting the bootloader in plain assembly... But to be honest, I'm too lazy for that... 😅

too lazy not crazy
Surely it can be done, but it sounds really time consuming and rather hard to debug (and contribute to).

I actually use the C ISA extension so the bootloader is always small enough, but I guess I'll change the default and then the user can enable it if needed. I would actually suggest changing the warning as well and recommend recompiling the bootloader for each processor to enable a potentially smaller bootloader, what do you think?

While writing to the TWI memory via the bootloader is not planned, I will probably upload a Python script to flash via an USB I2C adapter.

I actually use the C ISA extension so the bootloader is always small enough

That's a good solution! But I would recommend to provide the pre-built bootloader image compiled with the minimal ISA options only - so it is compatible to any ISA configuration.

I would actually suggest changing the warning as well and recommend recompiling the bootloader for each processor to enable a potentially smaller bootloader, what do you think?

Which warning?

Sure, we could add a note/tip to the according section of the documentation for this.

Btw, we have already enabled link-time-optimization and Os as compile-time switches. I am not sure if there is anything more we could do to further shrink the bootloader.

However, maybe it is worth a try to compile the bootloader with the absolute minimal ISA configuration which is rv32e. The E ISA extension only provides 16 x registers. Maybe the overhead of register spilling could be reduced by this?! 🤔

While writing to the TWI memory via the bootloader is not planned, I will probably upload a Python script to flash via an USB I2C adapter.

I think we could re-use parts of the "SPI write" code?

I just scanned your code briefly, but I think you are using a fixed number of address bytes for accessing the TWI flash, right? Maybe we could also make this configurable just like we did for the SPI accesses.

That's a good solution! But I would recommend to provide the pre-built bootloader image compiled with the minimal ISA options only - so it is compatible to any ISA configuration.

I agree, the pre-built should be compatible with any (but E currently) ISA configuration. But we could encourage the user to build the bootloader to their needs and ISA.

Which warning?

This one in bootloader.c:

/**********************************************************************//**
 * Sanity check: Base RV32I ISA only!
 **************************************************************************/
#if defined __riscv_atomic || defined __riscv_a || __riscv_b || __riscv_compressed || defined __riscv_c || defined __riscv_mul || defined __riscv_m
  #warning In order to allow the bootloader to run on *any* CPU configuration it should be compiled using the base rv32i ISA only.
#endif

But actually I'm fine with it, because the first part says to allow, which actually implies if you need it...

Btw, we have already enabled link-time-optimization and Os as compile-time switches. I am not sure if there is anything more we could do to further shrink the bootloader.

Probably not? -Oz is even more aggressive, but is currently the same size as -Os.

However, maybe it is worth a try to compile the bootloader with the absolute minimal ISA configuration which is rv32e. The E ISA extension only provides 16 x registers. Maybe the overhead of register spilling could be reduced by this?!

I'll look into it. This would also have the advantage that the bootloader would actually work on all ISA configurations.

I think we could re-use parts of the "SPI write" code?

Probably yes? Unfortunately I don't have a use case at the moment, but I keep it in mind. Probably not in this PR though. Some other bootloader extensions have a higher priority, you'll see :)

I just scanned your code briefly, but I think you are using a fixed number of address bytes for accessing the TWI flash, right? Maybe we could also make this configurable just like we did for the SPI accesses.

I agree, I'll add that one!

I agree, the pre-built should be compatible with any (but E currently) ISA configuration. But we could encourage the user to build the bootloader to their needs and ISA.

👍

This one in bootloader.c:

Oh, right. I forgot about that...

I think we could remove that. It does not capture all possible ISA extensions. So basically it is useless.

Probably not? -Oz is even more aggressive, but is currently the same size as -Os.

Interesting! I was not aware of that option. I'll look it up.

I'll look into it. This would also have the advantage that the bootloader would actually work on all ISA configurations.

I just tested that and it saves more than 100 bytes (yeay!).

-> #1118

I agree, I'll add that one!

Cool, thanks!

Tested with standard 16kbit one address EEPROM and 64kbit two address EEPROM. Also now with a python upload script but only for the USB-ISS module.

Unfortunately tested only before latest merge, so on main commit a0f4349 and not current upstream.

Seems to work on the newest version as well :) only tested without clint/autoboot and only on the two-address-byte eeprom but I haven't noticed any relevant commits anyways.

This looks great, thank you very much!
I'll have a close look at the code (unfortunately, I do not have a compaitble I²C flash...).

However, I'm not sure about the EEPROM tool. It is a cool feature and a handy tool, but this seems to be quite application-specific, right? 🤔

unfortunately, I do not have a compaitble I²C flash...

I suppose there could be FPGA models as well, but no additional testing would be required on my part.

However, I'm not sure about the EEPROM tool. It is a cool feature and a handy tool, but this seems to be quite application-specific, right?

It kind of is. It should work with all kinds of I2C chips, but yeah, it's only for a weirdly specific USB adapter. I'll remove it from the branch then.

I suggest renaming this to TWI_DEVICE_ID to match the phrases used by the other parts of the code.

I agree :)

Now the renaming should have an end, sorry for alle the different commits :) feel free to squash

This looks great! I just have some minor comments. 😉

By the way... How about also adding an option to store an image to TWI memory (similar to the SPI programming option)?

This looks great! I just have some minor comments. 😉

Thanks! And also thanks for the suggestions.

By the way... How about also adding an option to store an image to TWI memory (similar to the SPI programming option)?

I actually already answered this (#1108 (comment)), but no worries. I don't have a use case right now, but I'm keeping it in mind. Probably not in this PR though.